-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Plugin] Fix "Installing" bug when install plugin from Gemfile #6643
Conversation
Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack. For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this, I added a couple of comments.
Also, the previous CI timeout is unrelated to this PR and now fixed on master, so a rebased CI run should not run into that.
bundler/lib/bundler/plugin/index.rb
Outdated
@@ -111,6 +111,17 @@ def command_plugin(command) | |||
@commands[command] | |||
end | |||
|
|||
# Plugin cannot be installed and updating to install | |||
def cannot_install?(name, version) | |||
installed?(name) && !updating?(name, version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cannot_install?
makes me think of same problem when trying to install the gem. Maybe version_already_installed?
or something like that would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really sounds better, I changed the def name. Also want ask about the plugin version, do you have any suggest to how get it without using the regex, maybe a build-in way that I missed... or this way is fine? I'm talking about plugin index here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought but I could not find a better way that the regex based approach you used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Still the implementation and comment feels a bit confusing to me. I don't understand the !updating?(name, version)
part. How about
def version_already_installed?(name, version)
plugin_path = @plugin_paths[name]
return false unless plugin_path
version.to_s != plugin_path[/#{name}-(.*)$/, 1].to_s
end
Actually, would File.basename(plugin_path) == "#{name}-#{version}"
work instead of the regex based approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial objective was separate the methods to looks simple, because already exists the installed?
so I just added the !updating?
, maybe change to not_updating?
or same_version?
feels more readable. But since still sounds confuse, I can try your approach. Also, good idea about the "regex issue", I'll test :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, from the options you give I think same_version?
would be the best name for what we're checking here. I think better names would be something like some_version_installed?
instead of installed?
and same_version_installed?
instead of !updating?
. But since the second check gives really a subset of the first one, I think it's easier to inline the logic, hence my suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was testing your approach about the "regex issue", I found few more things to fix, and now looks like *almost everything works fine. I notice that the index file was not updating the path correctly after change versions updating/downgrading so I needed fix it too.
Almost because before, the plugin was suppose to appears "Using foo x.y.z" when already installed, but now is suppressed by default (only for plugins), I'm not sure if is a big problem. That happens because when you cli bundle install, the installed_specs returns the plugin while bundle by gemfile doesnt and I couldnt understand why that happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work, I'll loop back and have a look at that last issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @deivid-rodriguez !
I take a look again today on currently bundler version, looks like the updates solve the "using..." messages that I told before, what is nice, but installing messages and index file issue continues. For now I create a separate matcher because I thought that was causing the failures on specs, let's see if solves it :)
041a153
to
4a11628
Compare
839c753
to
bb22ccd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you haven't done anything that prevents downloading metadata from rubygems if the plugin is already installed
plugin_path = @plugin_paths[name] | ||
return false unless plugin_path | ||
|
||
File.basename(plugin_path) == "#{name}-#{version}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work for a plugin "my_plugin", path: "../my_plugin"
in your Gemfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ccutrer ! Indeed, you're right, I was focusing my attention on the "avoid unnecessary installs" problem and forgot about more things that I could fix. I saw your approach, better solving the problem at the root, congrats.
bb22ccd
to
9d86879
Compare
Since everyone seems to agree #6957 is a better approach, let's close this in favor of that other PR. |
What was the end-user or developer problem that led to this PR?
The problem was initially described here. When you install a plugin from a Gemfile (without git or path options), the bundle keep installing it every bundle install run.
What is your fix for the problem, implemented in this PR?
There is 2 problems to solve.
The first problem is block an installed plugin to install again, it was fixed the by adding a condition here, where I confirm if there is any plugin not installed yet before continue to install dependences.
The second problem is when you already have an installed plugin, but add other into your Gemfile, then it keep installing the old one. For this case, I need go a bit far to check if the plugin is installed and if the plugin installed version is the same. For this, I create a
cannot_install?
method on Index class for this propose. You can check the code here.Make sure the following tasks are checked